New User Setup and Input Overlay#722
Conversation
Repost of this commit since it was reverted. The Debug Settings system provides runtime-configurable debug features through a UI overlay. Settings are persisted to config.debug and can be accessed anywhere via the DebugSettings singleton. Also provide a way to generate the debug menu and surface it from a debug button. To support this new button and future overlays, add a root element to the game so we can order overlays Updated Bool Selector to size correctly Updated Style Selector creation to make sure sizing and layout is right Reload main menu when you come back to it Make navigation stack a UIElement so it works with touch handling Fixes panel-attack#697
Language selection setup screen created, shown when language not set Discord information setup screen created, new config for when it has been shown New Input Overlay Scene for picking inputs New controller theme graphics for representing input device type Comprehensive BattleRoom methods for managing players and their input assignments Restore previous assignments in 1P if they have been picked this session Language now defaults to English but not confirmed until picked Set Language method decoupled from saving language to support above InputPromptRenderer for drawing device images and eventually button prompts InputConfiguration has become a full class with device name, image, label etc fully accessible. InputManager manages the input configurations and provides a touch one. Added a second WASD default configuration for when new players don't have any yet and expect to navigate menus with WASD Input Device Overlay is shown in character select when not all human local players are assigned. Added change input device button for changing after you have already assigned Input Config Menu visuals improved, added auto configure joystick Added SceneCoordinator class for auto showing next scenes based on state Added ability for labels to autosize Added ability for menus to size to fit Added a slider menu item Made UIElement properly traverse touch from topmost down Auto show input config on new controller Add way to set TYPE variable on class constructor Added debugging print functions on UIElement Clear previous bindings when reusing key Require all keys bound before exiting config Input manager cleanup
|
Hi, some initial feedback, I hope to make some more serious progress over the weekend but there's something I already want to discuss regarding the architecture adjustment to the scenes. I wanted to review some yesterday and then got stuck superhard on this really early which kept me from looking at the rest so I'll get this one out early In particular this is about Obviously it works and it's good in the sense that the scenes themselves don't have to know about how they are used but I think the approach strays a bit too far from what a Scene ideally should be to keep things easily composable without having to add a class like SceneCoordinator to glue things together. Semantically I understand a Scene as the composition of audio, UI and input options that serves a specific purpose. For the feature in this PR I would intuitively think that what is currently I think having a I think we have two options here:
local startUpSequence = {}
-- Check language selection
if not config.language_code then
table.insert(startUpSequence, require("client.src.scenes.LanguageSelectSetup"))
end
-- Check Discord community welcome
if not config.discordCommunityShown then
table.insert(startUpSequence, require("client.src.scenes.DiscordCommunitySetup"))
end
-- Check for unconfigured joysticks
if input.hasUnsavedChanges or input:hasUnconfiguredJoysticks() then
table.insert(startUpSequence, require("client.src.scenes.InputConfigMenu"))
end
table.insert(startUpSequence, require("client.src.scenes.MainMenu"))
GAME.navigationStack:pushSequence(startUpSequence)The new I think I'm fine with either, I think you were leaning towards menus as UI elements before? |
|
I think I see what you mean, here is a proposal for a cleaner approach. Please let me know if its what you were thinking. Instead of having a separate SceneCoordinator class that works alongside the navigation system, I'll rename the BootScene to StartupScene and move all the SceneCoordinator logic into it.
The flow would work like this: StartUpScene enters and evaluates what's needed, pushes the first required setup scene, then when that scene pops, StartUpScene:resume() re-evaluates and either I may need to add a little bit of logic to NavigationStack so we can transition to the next scene without showing the boot scene for a frame and getting a flicker. TBD For the input config callback you mentioned should live on Game, I'll move that there or just call GAME.navigationStack:push(InputConfigMenu) directly from the startup scene. This eliminates having a coordinator side mechanism and generic callback that were the core concerns, while still giving us a proper place to orchestrate the first-time setup flow. Let me know if this works for you and I can implement. |
|
After attempting to implement the solution we discussed, I encountered a fundamental issue that I believe I encountered during my initial attempt. We require a seamless transition from Discord to InputConfig, with no flickering animation. This becomes even more challenging when we want to implement a slide transition or something similar, as both scenes are simultaneously visible. We cannot have an “in-between” scene that is not part of the actual transition. Instead, we need to replace one scene with another without briefly displaying something in between. BattleRoom, ChallengeMode, and Game all already coordinate scenes that perform similar functions. Additionally, we have the following requirements:
This is why I implemented the solution in the manner I did and added Scene.triggerNextScene to decouple the components. However, I believe triggerNextScene should not be generic, and each scene should have its own specific requirements for the name or if a function is required for returning. For instance, ChallengeMode could have a win and a lose function passed in. Therefore, I believe the original solution is largely sound, except for the fact that triggerNextScene should not be generic. Please advise whether you believe modifying that is sufficient or if we still need to make further changes. If so, how can we achieve these goals in an alternative manner? Alternatively, would you like to attempt refactoring while still meeting these goals? |
…nce within the BootScene
remove return from updating the cache, just call the getter instead in the one location it is required
… with the config global
…r array as its argument
Endaris
left a comment
There was a problem hiding this comment.
I did make 2 PRs on your branch on your repo.
One for the thing discussed previously.
The other is addressing the comments made for this review section so that it's a bit easier to parse individually why each change was made. I tried to make my changes commit by commit so you can easily revert some if you disagree.
I'm not quite done looking at everything but the remaining parts are mostly the new screens/UI elements which should be a bit faster as I'll have less preconceived ideas about them.
client/src/ui/UIElement.lua
Outdated
| function UIElement:debugTree() | ||
| local function getElementInfo(element, depth) | ||
| local indent = string.rep(" ", depth) | ||
| local typeStr = element.TYPE and (" [" .. element.TYPE .. "]") or "" | ||
| local x, y = element:getScreenPos() | ||
| local info = string.format("%s%s @ (%.1f, %.1f)", indent, typeStr, x, y) | ||
|
|
||
| local lines = {info} | ||
| for _, child in ipairs(element.children) do | ||
| local childInfo = getElementInfo(child, depth + 1) | ||
| table.insert(lines, childInfo) | ||
| end | ||
|
|
||
| return table.concat(lines, "\n") | ||
| end | ||
|
|
||
| return getElementInfo(self, 0) | ||
| end | ||
|
|
||
| ---Returns a formatted list of this element and its direct children only (non-recursive) | ||
| ---@return string | ||
| function UIElement:debugChildren() | ||
| local typeStr = self.TYPE and (" [" .. self.TYPE .. "]") or "" | ||
| local x, y = self:getScreenPos() | ||
| local lines = {string.format("%s @ (%.1f, %.1f)", typeStr, x, y)} | ||
|
|
||
| for _, child in ipairs(self.children) do | ||
| local childTypeStr = child.TYPE and (" [" .. child.TYPE .. "]") or "" | ||
| local childX, childY = child:getScreenPos() | ||
| table.insert(lines, string.format(" %s @ (%.1f, %.1f)", childTypeStr, childX, childY)) | ||
| end | ||
|
|
||
| return table.concat(lines, "\n") | ||
| end |
There was a problem hiding this comment.
Rename these to something like toStringWithDepth and toString?
common/engine/Match.lua
Outdated
| local InputCompression = require("common.data.InputCompression") | ||
| local ReplayV3 = require("common.data.ReplayV3") | ||
| local MatchRules = require("common.data.MatchRules") | ||
| local DebugSettings = require("client.src.debug.DebugSettings") |
There was a problem hiding this comment.
Obsolete leftover that is no longer needed after we put in 292f5fd
| -- Intentional override | ||
| ---@diagnostic disable-next-line: duplicate-set-field | ||
| function love.joystickadded(joystick) | ||
| GAME:onJoystickAdded(joystick) | ||
| end | ||
|
|
||
| -- Intentional override | ||
| ---@diagnostic disable-next-line: duplicate-set-field | ||
| function love.joystickremoved(joystick) | ||
| inputManager:onJoystickRemoved(joystick) | ||
| end |
There was a problem hiding this comment.
A bit unintuitive that one is routed through GAME and the other only through inputManager.
I see why but it might be consistent to just proxy joystickremoved through GAME as well.
client/src/config.lua
Outdated
| local encoded = json.encode(config) | ||
| ---@cast encoded string | ||
| love.filesystem.write("conf.json", json.encode(config)) |
There was a problem hiding this comment.
Assume you wanted to pass encoded in here as an argument?
| -- Intentional override | ||
| ---@diagnostic disable-next-line: duplicate-set-field | ||
| function love.joystickremoved(joystick) | ||
| -- GUID identifies the device type, 2 controllers of the same type will have a matching GUID | ||
| -- the GUID is consistent across sessions | ||
| local guid = joystick:getGUID() | ||
| -- ID is a per-session identifier for each controller regardless of type | ||
| local id = joystick:getID() | ||
|
|
||
| local vendorID, productID, productVersion = joystick:getDeviceInfo( ) | ||
|
|
||
| logger.info("Disconnecting device " .. vendorID .. ";" .. productID .. ";" .. productVersion .. ";" .. joystick:getName() .. ";" .. guid .. ";" .. id) | ||
|
|
||
| if joystickManager.guidsToJoysticks[guid] then | ||
| joystickManager.guidsToJoysticks[guid][id] = nil | ||
|
|
||
| if tableUtils.length(joystickManager.guidsToJoysticks[guid]) == 0 then | ||
| joystickManager.guidsToJoysticks[guid] = nil | ||
| end | ||
| end | ||
|
|
||
| joystickManager.devices[id] = nil | ||
| end |
There was a problem hiding this comment.
Good idea to not have the love callback here.
But logically this still should be a function on joystickManager just like the register function.
common/lib/joystickManager.lua
Outdated
| if joystickManager.devices[joystick:getID()] then | ||
| return | ||
| end |
There was a problem hiding this comment.
I think I prefer checking the inverse outside the function to make the code calling this more clear.
Otherwise, everytime I check joystickPressed I'll wonder "why is the joystick being registered on every single press?"
client/src/inputManager.lua
Outdated
| return result | ||
| end | ||
|
|
||
| function inputManager:write_key_file() |
There was a problem hiding this comment.
Rename to match our naming conventions if it's already being moved
There was a problem hiding this comment.
There are several uses of config as local variables / parameters.
This can be a bit confusing with shadowing our config global, so I would rename them to inputConfig just to be explicit.
client/src/inputManager.lua
Outdated
| ---@param battleRoom BattleRoom? | ||
| ---@return boolean True if an unassigned configuration has active input | ||
| function inputManager:checkForUnassignedConfigurationInputs(battleRoom) | ||
| if not battleRoom then | ||
| return false | ||
| end | ||
|
|
||
| local activeConfig = self:detectActiveInputConfiguration() | ||
| if not activeConfig then | ||
| return false | ||
| end | ||
|
|
||
| local assignedConfigs = {} | ||
| for _, player in ipairs(battleRoom:getLocalHumanPlayers()) do | ||
| if player.inputConfiguration then | ||
| assignedConfigs[player.inputConfiguration] = true | ||
| end | ||
| end | ||
|
|
||
| return not assignedConfigs[activeConfig] | ||
| end |
There was a problem hiding this comment.
Given that this function does not directly depend on battleRoom and the only place it is called already guarantees there is a battleRoom, it should take the output of battleRoom:getLocalHumanPlayers() directly as a parameter.
…ion from "press" to "hold" as short presses don't select the device
…tion and make them aware that contributions are possible
fix config not saving
…ear part of its core functionality relieve CharacterSelect/BattleRoom of some glue code that could live on Player/ChangeInputButton instead
…aving it spring up on its own
…hrough BattleRoom
…r/Input components
… is unclear during receipt of matchStart message
…restrict that way it is more likely that the inputMethod matches for a lastUsedInputConfig recovery in case of a matchStart
fixed PuzzleMenu not opening the inputoverlay
…languageselectsetup screen
Video showing language picker, discord info, configuring new controller, can't exit till not partially configured.
Screen.Recording.2025-10-31.at.9.20.53.AM.mp4